Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add debugging facilities to RuleNode/TokenNode exposed via N-API #908

Merged
merged 7 commits into from
Apr 11, 2024

Conversation

Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Mar 27, 2024

Closes #466

JSON serialization

Adds the toJSON method to the RuleNode/TokenNode classes with the comment stating that it's unstable and for debugging purposes only. I explicitly didn't type it (it's a string) as I didn't want to publicly encode any assumptions about the shape of the data.

We can discuss further what we want to explore as part of having pure data external interface (JSON) for Slang, whether that's exporting or importing JSON, but I'd consider this a separate issue as it requires more thought and design; the original issue, from what I understand, was primarily about improving the debugging/inspection experience.

Debugger

Adds hidden getter properties that is only eagerly evaluated by debugger; since it's a getter function, it shouldn't add overhead to the runtime but I have to admit, I didn't double-check that with numbers. It adds both __children and __text, allowing to quickly explore the tree structure recursively and also the enclosing text scope of the node:

vscode-js-debug (built-in to VS Code)

image

Chrome DevTools for Node

image

@Xanewok Xanewok requested a review from a team as a code owner March 27, 2024 11:01
Copy link

changeset-bot bot commented Mar 27, 2024

🦋 Changeset detected

Latest commit: eb63cda

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@nomicfoundation/slang Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Xanewok
Copy link
Contributor Author

Xanewok commented Mar 27, 2024

Custom formatting

I also went down the rabbit hole of trying to leverage custom inspection formatters like nodejs.util.inspect.common property for objects, here's the branch: https://github.com/Xanewok/slang/tree/cst-napi-debug-inspect.

In DevTools, we'd need to modify the global window.devtoolsFormatters, so without a dedicated extension this seems like a no-go.

In vscode-js-debug, they allow to customize the default header for an object but with caveats:

Future work

Having above in mind, we could, with increasing level of work required:

  1. always inject the custom formatter ourselves at Rust -> JS conversion (what's implemented in my branch above)
    a. wasteful; it'd be better to define it once at the class prototype level when registering the Node native addon rather than for every node conversion
    b. hacky; that's quite a bit of explicit lower-level N-API machinery involved
  2. Implement the support upstream for custom formatters or Symbol-keyed properties
    a. more optimal and declarative
    b. may be more helpful when printf-style debugging with util.inspect
    c. however, still not as useful in the actual debugger, as the custom preview is limited to depth = 1
  3. Somehow convince the folks responsible for vscode-js-debug to allow for stringified props of arbitrary depth
    a. there's probably more process and back-and-forth involved
    b. they probably added it for a reason (performance?); we'd probably have to opt-in in yet another way to this behavior
    c. however, the end result would resemble our interactive CST snapshots, which would be great for exploration and debugging

Overall, I think that just having the hidden, eagerly evaluated children and unparsed text vastly improves on what we had before and having the custom formatting would be a great bonus but is not required for good enough debugging story.

I would probably not pursue the remaining options for now as we have enough to work on in separate issues but if the napi-rs/vscode-js-debug support story changes, it'd be good to revisit this in the future.

@AntonyBlakey @OmarTawfik what do you guys think?

@Xanewok
Copy link
Contributor Author

Xanewok commented Apr 10, 2024

I've dropped the "unstable" mention for the JSON serialization per our f2f and I've clarified that "eager evaluation" is specifically for the inspected parent rather than in general (make it more clear that it's not recursive in nature by default).

@Xanewok Xanewok requested a review from OmarTawfik April 10, 2024 09:52
Copy link
Collaborator

@OmarTawfik OmarTawfik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few suggestions about missing catch_unwind and left over comment.

Otherwise, LGTM!

It seems these are not properly propagated if the inner called function
already uses the catch_unwind attribute, so we want to be extra careful
here.
@Xanewok Xanewok enabled auto-merge April 11, 2024 09:19
@Xanewok Xanewok added this pull request to the merge queue Apr 11, 2024
Merged via the queue into NomicFoundation:main with commit ab3688b Apr 11, 2024
1 check passed
@Xanewok Xanewok deleted the cst-napi-debug branch April 11, 2024 09:32
@github-actions github-actions bot mentioned this pull request Apr 11, 2024
github-merge-queue bot pushed a commit that referenced this pull request Apr 12, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and publish to npm
yourself or [setup this action to publish
automatically](https://github.com/changesets/action#with-publishing). If
you're not ready to do a release yet, that's fine, whenever you add more
changesets to main, this PR will be updated.


# Releases
## @nomicfoundation/slang@0.14.0

### Minor Changes

- [#753](#753)
[`b35c763`](b35c763)
Thanks [@AntonyBlakey](https://github.com/AntonyBlakey)! - Add tree
query implementation as `Query::parse` and `Cursor::query`

- [#755](#755)
[`8c260fc`](8c260fc)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support parsing
NatSpec comments

- [#908](#908)
[`ab3688b`](ab3688b)
Thanks [@Xanewok](https://github.com/Xanewok)! - Changed the
cst.NodeType in TS to use more descriptive string values rather than 0/1
integers

- [#886](#886)
[`0125717`](0125717)
Thanks [@Xanewok](https://github.com/Xanewok)! - Add
`TokenKind::is_trivia`

- [#887](#887)
[`dff1201`](dff1201)
Thanks [@Xanewok](https://github.com/Xanewok)! - Add support for
constant function modifier removed in 0.5.0

- [#885](#885)
[`a9bd8da`](a9bd8da)
Thanks [@Xanewok](https://github.com/Xanewok)! - Flatten the trivia
syntax nodes into sibling tokens

- [#908](#908)
[`ab3688b`](ab3688b)
Thanks [@Xanewok](https://github.com/Xanewok)! - Add
RuleNode/TokenNode::toJSON() in the TypeScript API

### Patch Changes

- [#801](#801)
[`ecbba49`](ecbba49)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - unreserve pragma
keywords in all versions

- [#869](#869)
[`951b58d`](951b58d)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support dots in
yul identifiers from `0.5.8` till `0.7.0`

- [#890](#890)
[`1ff8599`](1ff8599)
Thanks [@Xanewok](https://github.com/Xanewok)! - Mark `override` as
being a valid attribute only after 0.6.0

- [#800](#800)
[`d1827ff`](d1827ff)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support unicode
characters in string literals up to `0.7.0`

- [#797](#797)
[`86f36d7`](86f36d7)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - fix source
locations for unicode characters in error reports

- [#854](#854)
[`4b8970b`](4b8970b)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - parse line breaks
without newlines

- [#844](#844)
[`f62de9e`](f62de9e)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - fix parsing empty
`/**/` comments

- [#799](#799)
[`303dda9`](303dda9)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - prevent parsing
multiple literals under `StringExpression` before `0.5.14`

- [#847](#847)
[`6b6f260`](6b6f260)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - prioritize
parsing `MultiLineComment` over `MultiLineNatSpecComment`

- [#796](#796)
[`59e1e53`](59e1e53)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - add `public` and
`internal` to `UnnamedFunctionAttribute` till `0.5.0`

- [#756](#756)
[`e839817`](e839817)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - fix parsing
`payable` primary expressions

- [#851](#851)
[`67dfde8`](67dfde8)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - fix selection
order of prefix/postfix AST fields

- [#857](#857)
[`f677d5e`](f677d5e)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - rename
`FieldName` to `NodeLabel`

- [#852](#852)
[`ca79eca`](ca79eca)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - allow parsing
`ColonEqual` as two separate tokens before `0.5.5`

- [#889](#889)
[`ce5050f`](ce5050f)
Thanks [@Xanewok](https://github.com/Xanewok)! - Support `delete` as an
expression rather than a statement

- [#923](#923)
[`bb30fc1`](bb30fc1)
Thanks [@Xanewok](https://github.com/Xanewok)! - Support arbitrary ASCII
escape sequences in string literals until 0.4.25

- [#887](#887)
[`dff1201`](dff1201)
Thanks [@Xanewok](https://github.com/Xanewok)! - Support view and pure
function modifiers only from 0.4.16

- [#800](#800)
[`d1827ff`](d1827ff)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - rename
`AsciiStringLiteral` to `StringLiteral`

- [#838](#838)
[`ad98d1c`](ad98d1c)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - upgrade to rust
`1.76.0`

- [#849](#849)
[`5c42e0e`](5c42e0e)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - add `override`
and `virtual` to `ConstructorAttribute`

- [#862](#862)
[`5e37ea0`](5e37ea0)
Thanks [@Xanewok](https://github.com/Xanewok)! - allow call options as a
post-fix expression

- [#786](#786)
[`0bfa6b7`](0bfa6b7)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support Yul label
statements before `0.5.0`

- [#839](#839)
[`2d698eb`](2d698eb)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - support string
literals in version pragmas

- [#891](#891)
[`70c9d7d`](70c9d7d)
Thanks [@Xanewok](https://github.com/Xanewok)! - Fix parsing
`<NUMBER>.member` member access expression

- [#842](#842)
[`2069126`](2069126)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - add `private` to
`UnnamedFunctionAttribute` till `0.5.0`

- [#840](#840)
[`7fb0d20`](7fb0d20)
Thanks [@OmarTawfik](https://github.com/OmarTawfik)! - allow `var` in
`TupleDeconstructionStatement` before `0.5.0`

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NAPI Serialization for Nodes
2 participants